Skip to content

Data and functions 2#8

Open
montuckymike wants to merge 2 commits intoAmericaCampaign:masterfrom
montuckymike:data-and-functions-2
Open

Data and functions 2#8
montuckymike wants to merge 2 commits intoAmericaCampaign:masterfrom
montuckymike:data-and-functions-2

Conversation

@montuckymike
Copy link

No description provided.

Copy link
Contributor

@jcheroske jcheroske left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're doing great, Mike!

const getActiveUsers = (data) => {
if (data == null || data.users == null) {
return null
} else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can get rid of the else block and it will make your code easier to read.

} else {
const activeUsers = []
data.users.forEach((u) => {
if (u.accountActive === true) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since u.accountActive is a boolean, you can shorten your test to:

if (u.accountActive)

const getMostExpensiveProduct = (data) => {
if (data == null || data.products == null) {
return null
} else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This else block can be eliminated as well.

if (data == null || data.orders == null) {
return null
} else {
const orders = []
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we've already got data.orders, and since the function is called getOrderInto, lets call this array orderInfos instead.

if (data == null || data.products == null || id == null) {
return null
} else {
return data.products.find((p) => p.id === id)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When your lambda function only takes one parameter, you can eliminate the parens if you want. I find it makes the code easier to read:

p => p.id === id

Your call on that one though.

throw new Error('Do not have data or ID')
}
const order = data.orders.find((o) => o.id === id)
if (order === undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing for undefined is usually not recomended. It's more of a style thing. I would do one of these tests instead:

if (!order) {

or

if (order == null) {

@@ -0,0 +1,18 @@
import getProductById from './getProductById'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Notice how similar this function (getTotalPriceForOrder) is to getProductsForOrder. Can you import that function and use it to get the total price?

@jcheroske
Copy link
Contributor

You created your data-and-functions-2 branch from your data-and-functions-1 branch I think. That's why we see all of your answers, not just those from the second exercise. When branching, the new branch is a clone of the old one. In this case, you wanted to branch off of master instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Comments